-
Notifications
You must be signed in to change notification settings - Fork 73
Add support for pinging #77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I'm on vacation for a few weeks and won't be looking at this until I'm back. But the first thing I would ask is for there to be tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, I'd love to see some tests in https://github.com/aspnet/SignalR-Client-Cpp/blob/main/test/signalrclienttests/hub_connection_tests.cpp
README.md
Outdated
@@ -31,7 +31,7 @@ Below are instructions to build on different OS's. You can also use the followin | |||
```powershell | |||
PS> git submodule update --init | |||
PS> .\submodules\vcpkg\bootstrap-vcpkg.bat | |||
PS> .\submodules\vcpkg\vcpkg.exe install cpprestsdk:x64-windows | |||
PS> .\submodules\vcpkg\vcpkg.exe install cpprestsdk:x64-windows msgpack:x64-windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove, if you want you can add this to the -DUSE_MSGPACK
section in the table above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visual Studio shows errors regarding msg-pack then it try to parse cmakelists on folder-project opening. Maybe it is not an issue.
@@ -94,7 +94,7 @@ namespace signalr | |||
void reset() | |||
{ | |||
std::lock_guard<std::mutex> lock(m_lock); | |||
assert(m_callbacks.empty()); | |||
//assert(m_callbacks.empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion fails if I try to call start hub connection after it has disconnected. Is it okay to reuse hub connection instance or should I recreate it after disconnection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay to reuse hub connection instance or should I recreate it after disconnection?
Yes.
This assertion fails if I try to call start hub connection after it has disconnected.
That generally means something wasn't properly cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into my app code and prepare a test case and then propose a fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happened then hub_connection
disconnects on initial transport failure, for example then server is unavailable, and after that app calls start again, in my case this is a reconnect scenario, I have modified existing test to model this issue:
TEST(start, propogates_exception_from_negotiate_and_starts_again)
{
auto http_client = std::make_shared<test_http_client>([](const std::string& url, http_request, cancellation_token) -> http_response
{
throw custom_exception();
});
auto websocket_client = create_test_websocket_client();
auto hub_connection = hub_connection_builder::create("http://fakeuri")
.with_logging(std::make_shared<memory_log_writer>(), trace_level::none)
.with_http_client_factory([http_client](const signalr_client_config& config)
{
http_client->set_scheduler(config.get_scheduler());
return http_client;
})
.with_websocket_factory([websocket_client](const signalr_client_config&) { return websocket_client; })
.build();
auto mre = manual_reset_event<void>();
hub_connection.start([&mre](std::exception_ptr exception)
{
mre.set(exception);
});
try
{
mre.get();
ASSERT_TRUE(false);
}
catch (const custom_exception& e)
{
ASSERT_STREQ("custom exception", e.what());
}
hub_connection.start([&mre](std::exception_ptr exception)
{
}); ///<-------- here assertion fails
ASSERT_EQ(connection_state::disconnected, hub_connection.get_connection_state());
}
This happens because in the connection_impl::start_negotiate
, around 202 line, m_disconnect_cts
is not canceled like in the connection_impl::shutdown
.
I think I should create a separate issue on this matter because it is not directly connected with the issue we solving here.
@gonzo-coder are you planning on continuing with this change? |
@BrennanConroy yes! it was long holydays in my country... |
@BrennanConroy The tests and the code style fixes are ready. Please answer the question about an assert. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_websocket_client is going to need to ignore ping messages by default when invoking the test provided send_function otherwise some tests are going to occasionally fail.
@@ -1814,3 +1814,110 @@ TEST(send, throws_if_protocol_fails) | |||
|
|||
ASSERT_EQ(connection_state::connected, hub_connection->get_connection_state()); | |||
} | |||
|
|||
TEST(keepalive, sends_ping_messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have a test that a ping isn't sent if other messages are being sent from the hub_connection.
I have some test fixes in a local commit, but I think because you're using the "main" branch in your fork, I can't push any changes. Could you either add me as a collaborator to your fork or make a branch and recreate the PR from the branch? |
@BrennanConroy Added you as collaborator. Sorry I had holydays, no time to fix last issues. |
No worries, I had some free time and wanted to help out with some of the test issues. There are still a few comments that should be addressed, I can get to them in a few days if you don't have time. |
Thanks a lot @gonzo-coder ! This was super helpful 😃 |
Added Keep Alive Interval and Server Timeout Interval features as they are implemented in C# version of SignalR client. Solving of this issue.